Skip to content

Conversation

@Maffooch
Copy link
Contributor

@Maffooch Maffooch commented Nov 7, 2025

Enhance the tag processing logic in the DefaultImporter to correctly handle both lists and strings of tags. Introduce unit tests to verify the import functionality for reports containing tags.

Copy link
Member

@valentijnscholten valentijnscholten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. What is the reason this PR fixes the/a problem? Is it that finding.tags = clean_tags(finding.unsaved_tags) doesn't work but finding.tags.set( clean_tags(finding.unsaved_tags)) does?

@valentijnscholten valentijnscholten added this to the 2.52.1 milestone Nov 8, 2025
@valentijnscholten valentijnscholten changed the title Improve tag handling in DefaultImporter and add tests for tag imports Improve tag handling in importers and add tests for tag imports Nov 9, 2025
Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@Maffooch
Copy link
Contributor Author

What is the reason this PR fixes the/a problem? Is it that finding.tags = clean_tags(finding.unsaved_tags) doesn't work but finding.tags.set( clean_tags(finding.unsaved_tags)) does?

Yes. In previous versions, the tags were set before the finding was saved for the first time, so the tags would be applied normally. In today's version, the tags are set after the finding is saved, but not in the same way the other m2m relationships are managed. Since we only save the finding once in the importer now, we can't just set the tags attribute any longer. We have to use add/set in the m2m manager

@Maffooch Maffooch requested a review from rossops November 10, 2025 13:11
@rossops rossops merged commit d519cf7 into bugfix Nov 10, 2025
152 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants